Fix status bar color when a modal is open #9210

Tushu17 · 2022-05-28T18:37:20Z

Details

This PR fixes the different color issue of status bar when modal/popover is open in android

Fixed Issues

$ #8665

Tests

Open app in android (this is an android issue)
Go to payments and tap on an account(like shown in the screenshot) note: you can open any other modal/popover
Verify the status bar color isn't white and it matches with the overall screen

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Open app in android (this is an android issue)
Go to payments and tap on an account(like shown in the screenshot) note: you can open any other modal/popover
Verify the status bar color isn't white and it matches with the overall screen

Verify that no errors appear in the JS console

Screenshots

Note: It's an android only issue

Android

Web

Desktop

Mobile Web

Ios

rushatgabhane · 2022-05-29T21:12:05Z

@Tushu17 I get that it's an Android only issue, but we require contributors to post screenshots for all platforms before merging a PR (src: contributing.md)

Please ping me after that's done, thank you!!

Tushu17 · 2022-05-30T18:52:00Z

@rushatgabhane, done. I have attached the images.

rushatgabhane · 2022-05-30T19:14:20Z

@Tushu17 as per the screenshots you've added, the issue is reproducible on mWeb.

Can you fix that? This doc might be helpful - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta/name/theme-color

Tushu17 · 2022-05-30T20:24:00Z

Can you fix that?

@rushatgabhane, Sure we can change the theme-color by manually setting the required color when modals/popover appears.
To do that we can create a function which will use document object to change the meta content. Is this fine?

rushatgabhane · 2022-05-30T20:34:00Z

@Tushu17 in which file will this function be?

rushatgabhane · 2022-05-31T06:51:25Z

Btw, we change the title when a notification is received.
eg: New Expensify -> (1) New Expensify

You can see how that's done and solve this issue in a similiar way.

Tushu17 · 2022-05-31T19:57:55Z

@rushatgabhane , we can make a function like below:-

function changeStatusbarColor(isModalVisible) {
const meta = document.querySelector('meta[name="theme-color"]');
if (isModalVisible) {
meta.content = colors.gray2;
return;
}
meta.content = '';
}

We can add the 'meta[name="theme-color"]' in CONFIG.js as THEME_COLOR

inside willAlertModalBecomeVisible function we can call changeStatusbarColor(isVisible)

App/src/libs/actions/Modal.js

Lines 19 to 21 in bc571f3

    
           function willAlertModalBecomeVisible(isVisible) { 
        
               Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible}); 
        
           }

Can you suggest where should I be creating this function?

rushatgabhane · 2022-06-01T08:33:37Z

That seems like an anti pattern (modifying DOM elements directly), but there's proly no way around it.

Can you suggest where should I be creating this function?

@Tushu17 well, one way to do it would be under libs/ - setStatusBarColor(color)

Keep in mind that we don't wanna change the status bar color on Web (Safari)

rushatgabhane · 2022-06-01T09:07:22Z

@tgolen should we proceed with fixing this bug on mWeb too?

Summary of the proposed changes -

Add <meta> tag with name="theme-color" to web/index.html. (see mozilla docs)
Create a function setStatusBarColor(color) under libs/ (approx. function definition)
Set the appropriate status bar color on component mount and unmount in BaseModal.js

Caveats

We wanna keep web (Safari) unchanged. So gotta hook Modal to withWindowDimensions.
Modifying DOM elements directly is an antipattern.

mountiny

@rushatgabhane @Tushu17

I think that mWeb should be the lowest of our priorities as we want people to use the dedicated apps and optimize for those. As this solution would require quite an odd workaround, I think that just fixing this for Android is sufficient!

Everything seems good to me so I will wait for @rushatgabhane approval. Thanks for the work @Tushu17 and Rushat

rushatgabhane

@mountiny thanks for chiming in! 🎐
I agree, that makes sense!
was just exploring status bar colors for web because we could reuse the code if and when we add themes hehe

#### PR Reviewer Checklist

I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn’t already exist
- The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

mountiny · 2022-06-01T16:11:49Z

was just exploring status bar colors for web because we could reuse the code if and when we add themes hehe

I agree that is great to be on the lookout for!

OSBotify · 2022-06-01T17:34:28Z

🚀 Deployed to staging by @mountiny in version: 1.1.70-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-06-01T18:15:41Z

🚀 Deployed to staging by @mountiny in version: 1.1.70-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-06-01T18:53:41Z

🚀 Deployed to staging by @mountiny in version: 1.1.70-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

alex-mechler · 2022-06-01T21:51:49Z

This is causing a deploy blocker #9280

mountiny · 2022-06-01T22:51:06Z

The revert has been merged.

@Tushu17 Would you be able to look into a workaround to that regression? Thanks for any updates!

Tushu17 · 2022-06-02T07:03:35Z

At BaseModal.js we can add statusBarTranslucent: true in default props and we can just use statusBarTranslucent={this.props.statusBarTranslucent} here:-

App/src/components/Modal/BaseModal.js

Lines 79 to 81 in d2f9d61

    
           <ReactNativeModal 
        
               onBackdropPress={(e) => { 
        
                   if (e && e.type === 'keydown' && e.key === 'Enter') {

We have to add statusBarTranslucent={false} for the Modal below because we are having issues in this modal.

App/src/components/AttachmentModal.js

Lines 156 to 158 in d2f9d61

    
           <Modal 
        
               type={this.state.modalType} 
        
               onSubmit={this.submitAndClose}

cc: @mountiny @rushatgabhane

mountiny · 2022-06-02T15:15:36Z

@Tushu17 Thanks! Would you be able to provide screenshots of this solution and maybe point out why it is happening for that particular modal and not for others?

And also just to prevent jumping into any other regression - go through all the modals we have to make sure this is not happening for them too.

Tushu17 · 2022-06-03T15:01:50Z

@mountiny, when statusBarTranslucent is true the modal will go over the status bar and because the attachment modal covers the whole screen it was overlapping with the status bar so using the new approach we can just pass statusBarTranslucent={false} anywhere we don't want the modal to go over the status bar for example attachment modal.
I have tested other modals and didn't found any problem with them.

mountiny · 2022-06-03T15:11:27Z

@Tushu17 Thank you

I have tested other modals and didn't found any problem with them.

Lovely! Feel free to raise PR 🎉

OSBotify · 2022-06-06T17:38:26Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.71-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

change status bar color when modal is open

de86dfc

Tushu17 requested a review from a team as a code owner May 28, 2022 18:37

melvin-bot bot requested review from rushatgabhane and tgolen and removed request for a team May 28, 2022 18:37

mountiny self-requested a review June 1, 2022 15:50

mountiny approved these changes Jun 1, 2022

View reviewed changes

rushatgabhane approved these changes Jun 1, 2022

View reviewed changes

mountiny merged commit b38622d into Expensify:main Jun 1, 2022

OSBotify mentioned this pull request Jun 1, 2022

Deploy Checklist: New Expensify 2022-06-01 #9275

Closed

27 tasks

alex-mechler mentioned this pull request Jun 1, 2022

Android - Attachment - Header of the attachment overlaps with device status icons in the preview of the attachment #9280

Closed

mountiny mentioned this pull request Jun 1, 2022

Revert "Fix status bar color when a modal is open" #9284

Merged

SumitDiyora mentioned this pull request Jun 3, 2022

[High]: Chrome+JAWS: Screen reader: Role is not defined for controls. #8575

Closed

SumitDiyora mentioned this pull request Jun 3, 2022

[Medium]: Color Contrast: Insufficient color contrast for 'Delete' control. #8577

Closed

Tushu17 mentioned this pull request Jun 3, 2022

fix status bar color when modal/popover is open #9312

Merged

89 tasks

melvin-bot bot mentioned this pull request Jun 6, 2022

[HOLD for payment 2022-06-15] [$250] Android - Status bar color is different when a modal/popover is open @Tushu17 #8665

Closed

tienifr mentioned this pull request Mar 2, 2023

[HOLD for payment 2023-04-03] [$4000] Last message in the report shows for a brief moment when the keyboard opens while clicking on password text Input in password-protected pdf #15373

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix status bar color when a modal is open #9210

Fix status bar color when a modal is open #9210

Tushu17 commented May 28, 2022 •

edited

Loading

rushatgabhane commented May 29, 2022 •

edited

Loading

Tushu17 commented May 30, 2022

rushatgabhane commented May 30, 2022

Tushu17 commented May 30, 2022

rushatgabhane commented May 30, 2022 •

edited

Loading

rushatgabhane commented May 31, 2022

Tushu17 commented May 31, 2022

rushatgabhane commented Jun 1, 2022 •

edited

Loading

rushatgabhane commented Jun 1, 2022 •

edited

Loading

mountiny left a comment

rushatgabhane left a comment •

edited

Loading

mountiny commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

alex-mechler commented Jun 1, 2022

mountiny commented Jun 1, 2022

Tushu17 commented Jun 2, 2022

mountiny commented Jun 2, 2022

Tushu17 commented Jun 3, 2022 •

edited

Loading

mountiny commented Jun 3, 2022 •

edited

Loading

OSBotify commented Jun 6, 2022

Fix status bar color when a modal is open #9210

Fix status bar color when a modal is open #9210

Conversation

Tushu17 commented May 28, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Note: It's an android only issue

Android

Web

Desktop

Mobile Web

Ios

rushatgabhane commented May 29, 2022 • edited Loading

Tushu17 commented May 30, 2022

rushatgabhane commented May 30, 2022

Tushu17 commented May 30, 2022

rushatgabhane commented May 30, 2022 • edited Loading

rushatgabhane commented May 31, 2022

Tushu17 commented May 31, 2022

rushatgabhane commented Jun 1, 2022 • edited Loading

rushatgabhane commented Jun 1, 2022 • edited Loading

mountiny left a comment

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

#### PR Reviewer Checklist

mountiny commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

OSBotify commented Jun 1, 2022

alex-mechler commented Jun 1, 2022

mountiny commented Jun 1, 2022

Tushu17 commented Jun 2, 2022

mountiny commented Jun 2, 2022

Tushu17 commented Jun 3, 2022 • edited Loading

mountiny commented Jun 3, 2022 • edited Loading

OSBotify commented Jun 6, 2022

Tushu17 commented May 28, 2022 •

edited

Loading

rushatgabhane commented May 29, 2022 •

edited

Loading

rushatgabhane commented May 30, 2022 •

edited

Loading

rushatgabhane commented Jun 1, 2022 •

edited

Loading

rushatgabhane commented Jun 1, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

Tushu17 commented Jun 3, 2022 •

edited

Loading

mountiny commented Jun 3, 2022 •

edited

Loading